Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[P4Testgen] Replace absl::btree_map with the P4::flat_map implementation #4713

Merged
merged 1 commit into from
Nov 4, 2024
Merged

[P4Testgen] Replace absl::btree_map with the P4::flat_map implementation #4713

merged 1 commit into from
Nov 4, 2024

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Jun 6, 2024 •
edited
Loading

The flat map implementation was donated by @asl. It seems a little faster than btree_map.

@fruffy fruffy added the p4tools Topics related to the P4Tools back end label Jun 6, 2024
@fruffy fruffy marked this pull request as draft June 6, 2024 14:05
@asl
Copy link
Contributor

asl commented Jun 6, 2024

@fruffy I think we can jus grab existing ordered_map tests?

@asl
Copy link
Contributor

asl commented Jun 6, 2024

Added few tests and did some fixes here and there

@fruffy fruffy force-pushed the fruffy/flat_map branch 3 times, most recently from 2067229 to 7ba600c Compare June 12, 2024 14:16
@fruffy fruffy mentioned this pull request Jul 1, 2024
Merged
@fruffy fruffy force-pushed the fruffy/flat_map branch 4 times, most recently from 4099d76 to 9bc74fc Compare August 4, 2024 08:11
@fruffy fruffy mentioned this pull request Sep 10, 2024
Merged
@fruffy fruffy marked this pull request as ready for review October 25, 2024 15:56
@fruffy fruffy changed the title [P4Testgen] Replace boost::container::flat_map with a custom flat_map implementation [P4Testgen] Replace absl::btree_map with a custom flat_map implementation Oct 25, 2024
@fruffy fruffy changed the title [P4Testgen] Replace absl::btree_map with a custom flat_map implementation [P4Testgen] Replace absl::btree_map with a P4::flat_map implementation Oct 25, 2024
@fruffy fruffy changed the title [P4Testgen] Replace absl::btree_map with a P4::flat_map implementation [P4Testgen] Replace absl::btree_map with the P4::flat_map implementation Oct 25, 2024
@fruffy fruffy requested a review from vlstill October 31, 2024 18:09
vlstill
vlstill approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a sorted map better here than e.g. absl::flat_hash_map? Do we need it to be sorted?

@asl
Copy link
Contributor

asl commented Nov 4, 2024

Why is a sorted map better here than e.g. absl::flat_hash_map? Do we need it to be sorted?

IIRC the order did matter here. And there are lots of small maps here and there, so we need to reduce the overhead as well.

@fruffy
Copy link
Collaborator Author

fruffy commented Nov 4, 2024 •
edited
Loading

Why is a sorted map better here than e.g. absl::flat_hash_map? Do we need it to be sorted?

We do not have a hashing implementation for symbolic variables either. It is added here but not as efficient as it could be. #4667

@vlstill
Copy link
Contributor

vlstill commented Nov 4, 2024

We do not have a hashing implementation for symbolic variables either. It is added here but not as efficient as it could be. #4667

Yep, we don't need to squeeze all the last percents of performance in all case, I am fine with this using P4::flat_map, if in future it turns out absl::flat_hash_map would perform even better, we can always change it.

@fruffy fruffy added this pull request to the merge queue Nov 4, 2024
Merged via the queue into main with commit aa9bb95 Nov 4, 2024
18 of 19 checks passed
@fruffy fruffy deleted the fruffy/flat_map branch November 4, 2024 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlstill vlstill vlstill approved these changes

Assignees
No one assigned
Labels
p4tools Topics related to the P4Tools back end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fruffy @asl @vlstill